Update regenie to 3.6 #50468
Update regenie to 3.6 #50468
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces updates to the The patch file modifies the Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
recipes/regenie/patches/0015-update-cmakelist.txt-for-v3.6.patch (1)
15-21: Consider retaining build instructions for usersThe removal of the comments explaining how to set environment variables like
MKLROOT,OPENBLAS_ROOT, andSTATICmay make it harder for users to configure the build environment, especially if they are not familiar with the build process. These instructions are valuable for guiding users through custom build configurations.Consider keeping these comments or relocating them to documentation (e.g., README or INSTALL files) to assist users in setting up the build environment correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- recipes/regenie/meta.yaml (2 hunks)
- recipes/regenie/patches/0015-update-cmakelist.txt-for-v3.6.patch (1 hunks)
🧰 Additional context used
🔇 Additional comments (9)
recipes/regenie/meta.yaml (3)
2-2: Version update looks good.The version has been correctly updated from 3.5 to 3.6, which aligns with the PR objectives.
Line range hint
30-85: Verify dependencies and test commands for the new version.The build number is correctly set to 0 for the new version. The requirements, test commands, and other metadata remain unchanged. While this is often appropriate for minor version updates, it's worth verifying that:
- All dependencies are still compatible with version 3.6.
- The test commands are still valid and sufficient for version 3.6.
- There are no new features or changes in v3.6 that require updates to the metadata, such as new license files or documentation links.
To assist with this verification, you could:
- Check the release notes for regenie v3.6 for any changes that might affect the conda recipe.
- Run the package build and test process in a conda environment to ensure everything works as expected with the new version.
27-28: Verify the new patch file.The patch file has been correctly updated for the new version. The old patch for v3.5 is commented out, and a new patch for v3.6 is added. However, we should ensure that the new patch file exists and is correctly referenced.
To verify the existence and correct referencing of the new patch file, you can run the following script:
#!/bin/bash # Description: Verify the existence and correct referencing of the new patch file. # Check if the new patch file exists if [ -f "patches/0015-update-cmakelist.txt-for-v3.6.patch" ]; then echo "New patch file exists." else echo "Error: New patch file does not exist." fi # Check if the old patch file is correctly commented out in meta.yaml if grep -q "# - patches/0014-update-cmakelist.txt-for-v3.5.patch" meta.yaml; then echo "Old patch file is correctly commented out." else echo "Error: Old patch file is not correctly commented out." fi # Check if the new patch file is correctly referenced in meta.yaml if grep -q "- patches/0015-update-cmakelist.txt-for-v3.6.patch" meta.yaml; then echo "New patch file is correctly referenced." else echo "Error: New patch file is not correctly referenced." firecipes/regenie/patches/0015-update-cmakelist.txt-for-v3.6.patch (6)
112-113: Addition of OpenMP supportThe inclusion of OpenMP enhances the application's performance by enabling parallel processing. The
find_packageandtarget_link_librariescommands are correctly implemented to include OpenMP support.
336-338: Confirm the necessity of theOPTIONALflag in the install commandThe
installcommand for theregenietarget now includes theOPTIONALflag. This means if theregenieexecutable is not built, the installation process will not report an error. Verify whether it is acceptable for the build process to proceed without installingregenie, or if theregenietarget should always be present after a successful build.If
regenieis expected to be always built and installed, removing theOPTIONALflag may help catch build issues earlier.
317-321: Update tofind_libraryforZLIB_LIBRARYThe search for the Zlib library has been simplified from:
find_library(ZLIB_LIBRARY libz.a z REQUIRED)to:
find_library(ZLIB_LIBRARY z REQUIRED)This change is appropriate as it relies on the standard library name
z, allowing CMake to find the correct library without specifying the static archivelibz.a.
346-352: Integration of versioning and packagingThe addition of
write_basic_package_version_fileand the inclusion ofCPackenhance the project's version management and distribution capabilities. This facilitates easier packaging and installation of the software.
275-279: Ensure consistent linking of MKL libraries across platformsThe handling of MKL library linking differs between Linux and Darwin platforms. On Linux, if
BUILD_STATICis true, the link options include"-Wl,--start-group"and"-Wl,--end-group"with-lgomp, while on Darwin, these options are omitted.Ensure that the MKL libraries are correctly linked on all supported platforms and that the differences in linker options are intentional and necessary.
If you need assistance in verifying the correct linker flags for MKL on different platforms, I can help generate the appropriate CMake configurations.
30-84:⚠️ Potential issueVerify the removal of environment variable checks for external libraries
The code removes checks for environment variables such as
BGEN_PATH,MKLROOT,HTSLIB_PATH, andOPENBLAS_ROOT, which were previously used to specify custom paths to external libraries. Ensure that the build system can locate these libraries without the need for explicit environment variables and that users still have a way to provide custom paths if necessary.To confirm the build system correctly locates the required libraries, and users can specify custom paths if needed, consider reviewing the updated documentation or build scripts.
| {% set version = "3.5" %} | ||
| {% set sha256 = "48dfe8efa7b8acb6771452e3b53008f97d4a5c1241d5dbc3380d994b70a07a8c" %} | ||
| {% set version = "3.6" %} | ||
| {% set sha256 = "bb935b196da08ef7b37dad9736022030a686ae84539c47c106de6096d4612e09" %} |
There was a problem hiding this comment.
💡 Codebase verification
SHA256 checksum verification failed.
The checksum could not be verified due to missing or incorrectly referenced patch files and the absence of the meta.yaml file. Please ensure that all patch files are present and correctly referenced, and that meta.yaml exists in the repository.
🔗 Analysis chain
Verify the SHA256 checksum.
The SHA256 checksum has been updated, which is expected for a new version. However, it's crucial to ensure that this new checksum is correct for version 3.6 of regenie.
To verify the checksum, you can run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the SHA256 checksum of the regenie v3.6 source tarball.
# Download the source tarball
curl -L https://github.com/rgcgithub/regenie/archive/v3.6.tar.gz -o regenie-3.6.tar.gz
# Calculate the SHA256 checksum
calculated_checksum=$(sha256sum regenie-3.6.tar.gz | awk '{print $1}')
# Compare with the checksum in the recipe
expected_checksum="bb935b196da08ef7b37dad9736022030a686ae84539c47c106de6096d4612e09"
if [ "$calculated_checksum" = "$expected_checksum" ]; then
echo "Checksum verification passed."
else
echo "Checksum verification failed."
echo "Expected: $expected_checksum"
echo "Calculated: $calculated_checksum"
fi
# Clean up
rm regenie-3.6.tar.gz
Length of output: 676
| +if(BUILD_SHARED_LIBS) | ||
| + set(BUILD_STATIC OFF CACHE INTERNAL "Dynamic compilation") | ||
| + set(Boost_USE_STATIC_LIBS OFF) | ||
| +else() | ||
| set(BUILD_STATIC ON CACHE INTERNAL "Static compilation") | ||
| + set(Boost_USE_STATIC_LIBS ON) | ||
| message( STATUS "Static compilation mode") |
There was a problem hiding this comment.
Ensure correct logic for build type determination
The condition for setting BUILD_STATIC has been changed from checking the environment variable STATIC to BUILD_SHARED_LIBS. This modifies how the build system determines whether to perform a static or dynamic build. Ensure that BUILD_SHARED_LIBS is properly defined in all build configurations and that this change does not unintentionally alter the build behavior.
Consider setting a default value for BUILD_SHARED_LIBS or adjusting the logic to handle cases where it might be undefined.
| +target_include_directories(regenie PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/src ${CMAKE_PREFIX_PATH}/include) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assess the impact of changing visibility from PRIVATE to PUBLIC
The visibility of include directories and linked libraries in target_include_directories and target_link_libraries has been changed from PRIVATE to PUBLIC. This change exposes internal headers and libraries to any project that links against regenie, which may not be intended if regenie is not designed to be used as a library.
If exposing these directories and libraries is not necessary, consider retaining PRIVATE visibility to encapsulate internal dependencies and prevent potential linkage issues in downstream projects.
Also applies to: 129-130, 207-207, 216-216, 225-225, 233-233, 242-242, 260-260, 268-268, 297-297, 305-305
| -if(EXISTS ${MKLROOT}) | ||
| +if(DEFINED MKLROOT) |
There was a problem hiding this comment.
Validate the change from EXISTS to DEFINED for MKLROOT
The condition checking for MKLROOT has been changed from if(EXISTS ${MKLROOT}) to if(DEFINED MKLROOT). This alters the behavior from verifying whether the MKLROOT directory exists to merely checking if the variable is defined. Ensure that this change does not lead to issues when MKLROOT is defined but points to a non-existent directory.
Consider combining both checks:
if(DEFINED MKLROOT AND EXISTS ${MKLROOT})This ensures that MKLROOT is both defined and points to a valid directory.
Update
regenie: 3.5 → 3.6recipes/regenie(click to view/edit other files)@rgcgithubThis pull request was automatically generated (see docs).